[SM6.10][LinAlg] Remove TargetType, allow TG accumulate - #8883
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The public API retains an ignored target-type argument, while release-note and runtime coverage remain incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates Shader Model 6.10 LinAlg accumulation to match the revised specification.
Changes:
- Removes
TargetTypefrom matrix-to-groupshared-memory accumulation. - Enables thread-group-scope
InterlockedAccumulate. - Updates lowering, validation, APIs, and tests.
File summaries
| File | Description |
|---|---|
utils/hct/hctdb.py |
Updates DXIL operation metadata. |
utils/hct/gen_intrin_main.txt |
Updates builtin signature. |
tools/clang/unittests/HLSLExec/LinAlgTests.cpp |
Updates execution shaders. |
tools/clang/test/SemaHLSL/hlsl/linalg/builtins/vector-array-errors.hlsl |
Updates diagnostic invocation. |
tools/clang/test/SemaHLSL/hlsl/linalg/builtins/unavailable_pre_sm610.hlsl |
Updates availability test. |
tools/clang/test/SemaHLSL/hlsl/linalg/builtins/stage-errors.hlsl |
Updates stage test. |
tools/clang/test/SemaHLSL/hlsl/linalg/builtins/matrixaccumulatetomemory/vector-array-ast.hlsl |
Updates vector AST expectations. |
tools/clang/test/SemaHLSL/hlsl/linalg/builtins/matrixaccumulatetomemory/unavailable_pre_sm610.hlsl |
Updates availability test. |
tools/clang/test/SemaHLSL/hlsl/linalg/builtins/matrixaccumulatetomemory/ast.hlsl |
Updates AST signature. |
tools/clang/test/SemaHLSL/hlsl/linalg/builtins/matrix-builtins-ast.hlsl |
Updates aggregate AST test. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-vs.ll |
Updates VS DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-raygeneration.ll |
Updates ray-generation DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-ps.ll |
Updates PS DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-node.ll |
Updates node DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-ms.ll |
Updates mesh DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-miss.ll |
Updates miss-shader DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-intersection.ll |
Updates intersection DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-hs.ll |
Updates hull DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-gs.ll |
Updates geometry DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-ds.ll |
Updates domain DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-cs.ll |
Updates compute DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-closesthit.ll |
Updates closest-hit DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-callable.ll |
Updates callable DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-as.ll |
Updates amplification DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-stage-anyhit.ll |
Updates any-hit DXIL signature. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-matrixaccumulatetomemory.ll |
Revises validation cases. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-groupshared-vector-memory.ll |
Updates valid vector-memory operation. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-groupshared-vector-memory-invalid.ll |
Updates invalid vector-memory operation. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/matrixaccumulatetomemory/vector-array.hlsl |
Updates vector codegen expectations. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/matrixaccumulatetomemory/nominal.hlsl |
Updates nominal codegen expectations. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/api/matrix-groupshared-vector-array.hlsl |
Updates API vector-array codegen. |
tools/clang/test/CodeGenDXIL/hlsl/linalg/api/matrix-class.hlsl |
Adds thread-group codegen coverage. |
tools/clang/lib/Headers/hlsl/dx/linalg.h |
Revises public accumulation overloads. |
lib/HLSL/HLOperationLower.cpp |
Removes target type during lowering. |
lib/DxilValidation/DxilValidation.cpp |
Removes target-type validation. |
lib/DXIL/DxilOperations.cpp |
Updates operation arity. |
include/dxc/DXIL/DxilInstructions.h |
Updates operand accessors and count. |
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // CHECK-SAME: float addrspace(3)* getelementptr inbounds ([256 x float], | ||
| // CHECK-SAME: [256 x float] addrspace(3)* @"\01?SharedArr@@3PAMA", i32 0, i32 0), i32 0, i32 16, i32 1) | ||
| // CHECK-SAME: ; LinAlgMatrixAccumulateToMemory(matrix,memory,offset,stride,layout) | ||
| TGAccMat.InterlockedAccumulate(SharedArr, 0, 16, MatrixLayoutEnum::ColMajor); |
There was a problem hiding this comment.
Doesn't need to be in this PR but would be good to make sure there's an issue tracking it.
There was a problem hiding this comment.
There was a problem hiding this comment.
🔵 Needs a closer look
The API retains an ignored target-type template argument, and the user-visible change lacks a preview release note.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tools/clang/test/CodeGenDXIL/hlsl/linalg/api/matrix-class.hlsl:179
- This adds user-visible experimental SM 6.10 behavior, but the PR does not add a release note. Please document the removed target-type operand and thread-group accumulation support under
### Upcoming Preview Releaseindocs/ReleaseNotes.md, as required by the repository's release-note policy.
tools/clang/lib/Headers/hlsl/dx/linalg.h:380
TargetCompTyis now unused, yet callers can still writeInterlockedAccumulate<ComponentType::I32>(as the updated test does). That explicit type is silently ignored and produces the same operation as the default overload, which can change the meaning of existing source without a diagnostic. Remove this template parameter and update explicit call sites so obsolete target-type requests are rejected rather than accepted as no-ops.
template <ComponentEnum TargetCompTy = ComponentTy, typename T,
MatrixUseEnum UseLocal = Use, SIZE_TYPE Size>
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Jesse Natalie (jenatali)
left a comment
There was a problem hiding this comment.
One bot comment worth fixing, otherwise LGTM.
|
LGTM. Sorry for the spec churn. |
There was a problem hiding this comment.
🟡 Changes recommended
Stale explicit template arguments break two tests, and the revised overloads incorrectly permit thread-scoped matrices that generate invalid DXIL.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tools/clang/lib/Headers/hlsl/dx/linalg.h:379
- This packed-memory overload also lost its scope guard, so a thread-scoped accumulator can now call it even though the DXIL validator rejects Thread scope for
LinAlgMatrixAccumulateToMemory. Restrict it to Wave and ThreadGroup as well.
template <typename T, MatrixUseEnum UseLocal = Use, SIZE_TYPE Size>
- Files reviewed: 37/37 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The public overloads now admit thread-scope matrices that the DXIL validator explicitly rejects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
tools/clang/lib/Headers/hlsl/dx/linalg.h:371
- Removing the scope predicate makes this overload available for
MatrixScope::Thread, but the DXIL validator accepts only Wave or ThreadGroup matrices (lib/DxilValidation/DxilValidation.cpp:1507-1512). Such a call now passes the HLSL API and emits invalid DXIL; retain a deferred scope constraint while adding ThreadGroup support.
Use == MatrixUse::Accumulator && UseLocal == Use,
tools/clang/lib/Headers/hlsl/dx/linalg.h:383
- This packed-memory overload also lost its scope restriction, so a thread-scope accumulator can select it even though
LinAlgMatrixAccumulateToMemoryrejects Thread scope during DXIL validation (lib/DxilValidation/DxilValidation.cpp:1507-1512). Restrict it to Wave or ThreadGroup just like the scalar/vector overload.
Use == MatrixUse::Accumulator && UseLocal == Use,
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The signature change is consistently propagated through the API, lowering, validation, instruction metadata, and relevant tests.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Implements the spec changes as defined by microsoft/hlsl-specs#927
dx.op.linAlgMatrixAccumulateToMemoryInterlockedAccumulateto gs memoryFixes: #8897
Co-authored-by: Copilot [email protected]